perf: optimize LEAD/LAG IGNORE NULLS evaluation#23711
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23711 +/- ##
========================================
Coverage 80.75% 80.75%
========================================
Files 1089 1089
Lines 368809 368941 +132
Branches 368809 368941 +132
========================================
+ Hits 297837 297945 +108
- Misses 53217 53232 +15
- Partials 17755 17764 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // `zip` concatenates dictionaries, which can overflow bounded key types. | ||
| if matches!(array.data_type(), DataType::Dictionary(_, _)) && !default_value.is_null() |
There was a problem hiding this comment.
is this a limitation of zip or a bug with it?
There was a problem hiding this comment.
This is a current implementation bug/limitation in Arrow’s zip/MutableArrayData dictionary path, rather than an inherent limitation of zip semantics. It concatenates the complete backing dictionaries and can panic on bounded-key overflow even when the selected output would fit. interleave already has specialized dictionary merging/re-encoding logic. I’ll add a bounded-dictionary regression test as well.
| let has_value = | ||
| is_not_null(&indices).map_err(|error| arrow_datafusion_err!(error))?; | ||
| let shifted: &dyn arrow::array::Array = shifted.as_ref(); | ||
| zip(&has_value, &shifted, &default_value.to_scalar()?) |
There was a problem hiding this comment.
i wonder if doing interleave instead of take + zip for the non-null default value path would be more efficient? or take+zip is plenty efficient enough 🤔
There was a problem hiding this comment.
Yes, direct interleave looks preferable here. Including index construction, it was about 1.55x faster for Int64, 1.41x for Utf8View, and 3.81x for List in my tested workload. I’ll switch the non-null-default path to build the interleave indices directly, avoiding the intermediate take result and subsequent zip.
613e6d3 to
6711e7c
Compare
3 participants
Which issue does this PR close?
Rationale for this change
The whole-partition evaluation path for
LEADandLAGwithIGNORE NULLScurrently collects every valid row index, performs a binary search for every output row, converts every selected value to aScalarValue, and finally rebuilds an Arrow array from those scalars.This makes index selection
O(n log m)fornrows andmnon-null rows, and the per-row scalar materialization is particularly expensive for strings and nested values.A local Criterion microbenchmark with 100,000 rows and 50% nulls measured the following speedups across the tested offsets:
Int64Utf8ViewListWhat changes are included in this PR?
VecDequestate instead of collecting all valid indices and binary-searching for every row.takekernel.zipkernel to fill non-null default values without constructing oneScalarValueper row.zipconcatenates dictionaries and can overflow bounded dictionary key types.The index generation and output materialization are now
O(n). There are no public API changes.Are these changes tested?
Yes.
Are there any user-facing changes?
No. This changes the implementation of whole-partition
LEAD/LAG ... IGNORE NULLSevaluation without changing the SQL behavior or public API.